Added part of the 'String Commands' group to RedisClient as specified on the official Redis website. #68

SanHalacogluImproving · 2024-01-25T19:32:20Z

No description provided.

jonathanl-bq · 2024-01-25T22:46:39Z

java/client/src/test/java/glide/api/RedisClientTest.java

@@ -318,4 +318,170 @@ public void set_withOptionsOnlyIfDoesNotExist_success()
        assertNotNull(response);
        assertEquals(value, response.get());
    }
+
+    @Test
+    public void decr_success() throws ExecutionException, InterruptedException {


Are we planning to add non-successful cases?

* Add base command; add custom command --------- Signed-off-by: Andrew Carbonetto <[email protected]>

Signed-off-by: Andrew Carbonetto <[email protected]>

… on the official Redis website.

java/client/src/main/java/glide/api/BaseClient.java

java/client/src/main/java/glide/api/RedisClient.java

Yury-Fridlyand · 2024-01-26T20:24:26Z

java/client/src/main/java/glide/api/RedisClient.java

+     *
+     * @see <a href="https://redis.io/commands/decr/">redis.io</a> for details.
+     * @param key - The key to decrement its value.
+     * @return the value of `key` after the decrement. An error is raised if `key` contains a value of


Suggested change

* @return the value of `key` after the decrement. An error is raised if `key` contains a value of

* @return The value of `key` after the decrement. An error is raised if `key` contains a value of

Keep the casing consistend across the docs (update below too)

java/client/src/main/java/glide/api/RedisClient.java

Yury-Fridlyand · 2024-01-26T20:28:45Z

java/client/src/main/java/glide/api/commands/StringCommands.java

 import java.util.concurrent.CompletableFuture;

-/** String Commands interface to handle single commands that return Strings. */
+/**
+ * String Commands interface.


Could be a bit confusing, becase string commands don't return strings actually.

Suggested change

* String Commands interface.

* Commands that perform string manipulation.

java/client/src/test/java/glide/api/RedisClientTest.java

Yury-Fridlyand · 2024-01-26T20:31:11Z

java/client/src/test/java/glide/api/RedisClientTest.java

I propose to leave here tests which required to provide test coverage, but all meaningful test move to IT, where you can execute a command without mocking anything.
Probably, wait for valkey-io#863 to merge or rebase on IT test branch.

Yury-Fridlyand · 2024-01-26T20:31:50Z

Please, also fix CI (again spotless, oh) and update PR description with examples and list of commands implemented

Yury-Fridlyand · 2024-01-26T21:13:48Z

java/client/src/main/java/glide/api/RedisClient.java

+     * @see <a href="https://redis.io/commands/incrby/">redis.io</a> for details.
+     * @param key - The key to increment its value.
+     * @param amount - The amount to increment.
+     * @returns the value of `key` after the increment, An error is raised if `key` contains a value


Important: the tag should be @return - please update everywhere

Suggested change

* @returns the value of `key` after the increment, An error is raised if `key` contains a value

* @return The value of `key` after the increment, An error is raised if `key` contains a value

Changed return type for MGET (from Object[] to String[]). Minor fixes in tests.

aaron-congo · 2024-01-29T17:16:01Z

java/client/src/main/java/glide/api/RedisClient.java

-     * @param msg - the ping argument that will be returned.
-     * @returns return a copy of the argument.
+     * @param msg The ping argument that will be returned.
+     * @return Return a copy of the argument.


nit: Seems like in the other docs we don't do "@return Return"

Suggested change

* @return Return a copy of the argument.

* @return A copy of the argument.

Suggested change

* @return Return a copy of the argument.

* @return A server response

aaron-congo · 2024-01-29T17:16:37Z

java/client/src/main/java/glide/api/RedisClient.java

+     * @param key The key to store.
+     * @param value The value to store with the given key.
+     * @param options The Set options
+     * @return String or null If value isn't set because of `onlyIfExists` or `onlyIfDoesNotExist`


nit: taking this from the python client docs:

Suggested change

* @return String or null If value isn't set because of `onlyIfExists` or `onlyIfDoesNotExist`

* @return The old value as a string if `returnOldValue` is set. Otherwise, if the value isn't set because of `onlyIfExists` or `onlyIfDoesNotExist`, return null. Otherwise, return "OK".

use {@link returnOldValue} (add proper link) instead of just `returnOldValue`

aaron-congo · 2024-01-29T17:18:37Z

java/client/src/main/java/glide/api/RedisClient.java

+     *
+     * @see <a href="https://redis.io/commands/incr/">redis.io</a> for details.
+     * @param key The key to increment its value.
+     * @return The value of `key` after the increment, An error is raised if `key` contains a value of


nit: this should be a period instead of comma, same thing applies for other occurrences in this PR

Suggested change

* @return The value of `key` after the increment, An error is raised if `key` contains a value of

* @return The value of `key` after the increment. An error is raised if `key` contains a value of

Yury-Fridlyand · 2024-01-31T01:27:39Z

java/client/src/main/java/glide/api/RedisClient.java

-     * @param args arguments for the custom command
-     * @return a CompletableFuture with response result from Redis
+     * @param args Arguments for the custom command
+     * @return A CompletableFuture with response result from Redis


As we discussed, you can simplify that - here and for all other functions (except CreateClient I think)

Suggested change

* @return A CompletableFuture with response result from Redis

* @return A response from Redis

Yury-Fridlyand · 2024-01-31T01:27:54Z

java/client/src/main/java/glide/api/RedisClient.java

@@ -92,7 +94,7 @@ public CompletableFuture<Object> customCommand(String[] args) {
     * Ping the Redis server.
     *
     * @see <a href="https://redis.io/commands/ping/">redis.io</a> for details.
-     * @returns the String "PONG"
+     * @return The String "PONG"


Suggested change

* @return The String "PONG"

* @return A server response

Yury-Fridlyand · 2024-01-31T01:28:05Z

java/client/src/main/java/glide/api/RedisClient.java

-     * @param msg - the ping argument that will be returned.
-     * @returns return a copy of the argument.
+     * @param msg The ping argument that will be returned.
+     * @return Return a copy of the argument.


Suggested change

* @return Return a copy of the argument.

* @return A server response

Yury-Fridlyand · 2024-01-31T01:28:38Z

java/client/src/main/java/glide/api/RedisClient.java

@@ -133,7 +135,7 @@ public CompletableFuture<Map> info() {
     * Get information and statistics about the Redis server.
     *
     * @see <a href="https://redis.io/commands/info/">redis.io</a> for details.
-     * @param options - A list of InfoSection values specifying which sections of information to
+     * @param options A list of InfoSection values specifying which sections of information to


Suggested change

* @param options A list of InfoSection values specifying which sections of information to

* @param options A list of {@link InfoSection} values specifying which sections of information to

Yury-Fridlyand · 2024-01-31T01:28:56Z

java/client/src/main/java/glide/api/RedisClient.java

@@ -133,7 +135,7 @@ public CompletableFuture<Map> info() {
     * Get information and statistics about the Redis server.
     *
     * @see <a href="https://redis.io/commands/info/">redis.io</a> for details.
-     * @param options - A list of InfoSection values specifying which sections of information to
+     * @param options A list of InfoSection values specifying which sections of information to
     *     retrieve. When no parameter is provided, the default option is assumed.
     * @return CompletableFuture with the response


Suggested change

* @return CompletableFuture with the response

* @return A server response

Yury-Fridlyand · 2024-01-31T01:32:25Z

java/client/src/main/java/glide/api/RedisClient.java

+        List<String> flatMap = new ArrayList<>();
+
+        for (Map.Entry<String, String> entry : keyValueMap.entrySet()) {
+            flatMap.add(entry.getKey());
+            flatMap.add(entry.getValue());
+        }
+
+        String[] args = flatMap.toArray(new String[0]);


you can avoid creating intermediate list for better performance

Yury-Fridlyand · 2024-01-31T01:32:51Z

java/client/src/main/java/glide/api/commands/StringCommands.java

 public interface StringCommands {

    CompletableFuture<String> get(String key);

    CompletableFuture<Void> set(String key, String value);

    CompletableFuture<String> set(String key, String value, SetOptions options);
+
+    CompletableFuture<Long> decr(String key);


move all javadocs from client to here - for all functions.

Yury-Fridlyand · 2024-01-31T01:33:24Z

java/client/src/test/java/glide/api/RedisClientTest.java

    @Test
-    public void set_withOptionsOnlyIfExists_success()
-            throws ExecutionException, InterruptedException {
+    public void set_withOptionsOnlyIfExists_success() {


please rename the test

Yury-Fridlyand · 2024-01-31T01:33:29Z

java/client/src/test/java/glide/api/RedisClientTest.java

    @Test
-    public void set_withOptionsOnlyIfDoesNotExist_success()
-            throws ExecutionException, InterruptedException {
+    public void set_withOptionsOnlyIfDoesNotExist_success() {


Yury-Fridlyand · 2024-01-31T01:33:45Z

java/client/src/test/java/glide/api/RedisClientTest.java

+
+    @SneakyThrows
+    @Test
+    public void decr_success() {


consider moving new tests to new file(s)

SanHalacogluImproving requested review from aaron-congo, jonathanl-bq, Yury-Fridlyand and acarbonetto January 25, 2024 19:32

SanHalacogluImproving self-assigned this Jan 25, 2024

jonathanl-bq reviewed Jan 25, 2024

View reviewed changes

acarbonetto and others added 5 commits January 25, 2024 16:15

Java - Add custom command interface; and BaseCommands (#58)

c1463e9

* Add base command; add custom command --------- Signed-off-by: Andrew Carbonetto <[email protected]>

Move Command resolvers to manager level

f9741b8

Signed-off-by: Andrew Carbonetto <[email protected]>

Spotless

888eac5

Signed-off-by: Andrew Carbonetto <[email protected]>

Initial commit for get/set/info/ping commands

5f14dba

Signed-off-by: Andrew Carbonetto <[email protected]>

Added part of the 'String Commands' group to RedisClient as specified…

c8a54bb

… on the official Redis website.

SanHalacogluImproving force-pushed the java/integ_SanH_StringCommands branch from df8b07d to 1dcc052 Compare January 26, 2024 00:21

SanHalacogluImproving force-pushed the java/dev_SanH_StringCommands branch from 6340d2f to c8a54bb Compare January 26, 2024 00:25

Yury-Fridlyand reviewed Jan 26, 2024

View reviewed changes

Yury-Fridlyand mentioned this pull request Jan 26, 2024

Add GET/SET/INFO/PING commands #66

Closed

Yury-Fridlyand reviewed Jan 26, 2024

View reviewed changes

SanHalacogluImproving added 7 commits January 26, 2024 15:26

Updated access modifiers in RedisClient to protected instead of public.

8db3768

Changed return type for MGET (from Object[] to String[]). Minor fixes in tests.

Minor fixes for tests and documentation.

74ad98e

Minor fix documentation.

0b1307d

Minor fix documentation.

65e7219

Minor fix documentation.

062647f

Minor fix documentation.

d4b6a73

Spotless.

e262018

aaron-congo approved these changes Jan 29, 2024

View reviewed changes

Yury-Fridlyand reviewed Jan 31, 2024

View reviewed changes

SanHalacogluImproving closed this Feb 6, 2024

SanHalacogluImproving deleted the java/dev_SanH_StringCommands branch February 6, 2024 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added part of the 'String Commands' group to RedisClient as specified on the official Redis website. #68

Added part of the 'String Commands' group to RedisClient as specified on the official Redis website. #68

SanHalacogluImproving commented Jan 25, 2024

jonathanl-bq Jan 25, 2024

Yury-Fridlyand Jan 26, 2024

Yury-Fridlyand Jan 26, 2024

Yury-Fridlyand Jan 26, 2024

Yury-Fridlyand commented Jan 26, 2024 •

edited

Loading

Yury-Fridlyand Jan 26, 2024

aaron-congo Jan 29, 2024

Yury-Fridlyand Jan 31, 2024

aaron-congo Jan 29, 2024

Yury-Fridlyand Jan 31, 2024

aaron-congo Jan 29, 2024

Yury-Fridlyand Jan 31, 2024

Yury-Fridlyand Jan 31, 2024

Yury-Fridlyand Jan 31, 2024

Yury-Fridlyand Jan 31, 2024

Yury-Fridlyand Jan 31, 2024

Yury-Fridlyand Jan 31, 2024

Yury-Fridlyand Jan 31, 2024

Yury-Fridlyand Jan 31, 2024

Yury-Fridlyand Jan 31, 2024

Yury-Fridlyand Jan 31, 2024

	* @return the value of `key` after the decrement. An error is raised if `key` contains a value of
	* @return The value of `key` after the decrement. An error is raised if `key` contains a value of

	* String Commands interface.
	* Commands that perform string manipulation.

	* @returns the value of `key` after the increment, An error is raised if `key` contains a value
	* @return The value of `key` after the increment, An error is raised if `key` contains a value

	* @return Return a copy of the argument.
	* @return A copy of the argument.

	* @return Return a copy of the argument.
	* @return A server response

	* @return String or null If value isn't set because of `onlyIfExists` or `onlyIfDoesNotExist`
	* @return The old value as a string if `returnOldValue` is set. Otherwise, if the value isn't set because of `onlyIfExists` or `onlyIfDoesNotExist`, return null. Otherwise, return "OK".

	* @return A CompletableFuture with response result from Redis
	* @return A response from Redis

	* @param options A list of InfoSection values specifying which sections of information to
	* @param options A list of {@link InfoSection} values specifying which sections of information to

	* @return CompletableFuture with the response
	* @return A server response

Added part of the 'String Commands' group to RedisClient as specified on the official Redis website. #68

Added part of the 'String Commands' group to RedisClient as specified on the official Redis website. #68

Conversation

SanHalacogluImproving commented Jan 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand commented Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yury-Fridlyand commented Jan 26, 2024 •

edited

Loading